Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

OpenAPI: Add RemoveSchemas REST update type #12022

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

OpenAPI: Add RemoveSchemas REST update type #12022

wants to merge 1 commit into from
+22 −0

Conversation

gaborkaszab
Copy link
Collaborator

No description provided.

@gaborkaszab
Copy link
Collaborator Author

Similarly to removing unused specs, I started working on removing unused schemas. This PR is similar to #10846
As an author of the other PR, @amogh-jahagirdar would you mind taking a look?

@gaborkaszab gaborkaszab force-pushed the main_rest_remove_schemas branch from 637f0bf to 89d177f Compare January 21, 2025 17:39
amogh-jahagirdar
amogh-jahagirdar reviewed Jan 22, 2025
View reviewed changes
Copy link
Contributor

@amogh-jahagirdar amogh-jahagirdar left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@gaborkaszab I agree with this spec change, I think it's very important to maintaining table metadata sizes for cases where there's frequent schema evolution but we'll need to put this to a discuss/vote thread.

Could you send an email on the dev list? https://lists.apache.org/thread/2lo59wkhn8cm29wmokfmg2f18934qnln as an example

@gaborkaszab
Copy link
Collaborator Author

but we'll need to put this to a discuss/vote thread.

Thanks for taking a look, @amogh-jahagirdar !
Yes, I was aware of the need of at least a [VOTE] thread, but wasn't sure about the order of reviewing the PR vs opening a mail thread. I started a [DISCUSS] thread now, and if that goes well I guess the [VOTE] is just formality.

@gaborkaszab gaborkaszab mentioned this pull request Jan 24, 2025
Open
@gaborkaszab
Copy link
Collaborator Author

cc @advancedxy

advancedxy
advancedxy reviewed Feb 8, 2025
View reviewed changes
Copy link
Contributor

@advancedxy advancedxy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for pinging me. I think this is valuable especially for wide tables which have frequent schema update.

BTW, I think we should start the discuss/vote to get REST spec change passed first.

open-api/rest-catalog-open-api.py
@@ -390,6 +390,11 @@ class RemovePartitionSpecsUpdate(BaseUpdate):
spec_ids: List[int] = Field(..., alias='spec-ids')


class RemoveSchemasUpdate(BaseUpdate):
action: Optional[Literal['remove-schemas']] = None
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why this is optional? Looks like all other actions are constant field.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@advancedxy advancedxy advancedxy left review comments

@amogh-jahagirdar amogh-jahagirdar amogh-jahagirdar left review comments

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
OPENAPI
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@gaborkaszab @advancedxy @amogh-jahagirdar